-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ImageBasePositionedComponent): add subcomponent to positioning component in Image #7166
base: master
Are you sure you want to change the base?
feat(ImageBasePositionedComponent): add subcomponent to positioning component in Image #7166
Conversation
…omponent in Image - Добавил сабкомпонент ImageBasePositionedComponent для абсолютного позиционирования компонентов в компоненте Image. - Добавил тесты для компонента - Добавил стори для компонента - Добавил использование компонента в документацию
size-limit report 📦
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
e2e tests |
👀 Docs deployed
Commit 537bc0e |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7166 +/- ##
==========================================
+ Coverage 95.14% 95.17% +0.02%
==========================================
Files 384 386 +2
Lines 11357 11426 +69
Branches 3727 3746 +19
==========================================
+ Hits 10806 10875 +69
Misses 551 551
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Выглядит здорово.
Я так понимаю, что он сразу сделан с возможностью использовать несколько Image.PositionedComponent
. 👍
Оставил пару комментариев.
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Шикарно 🔥
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
React.useEffect( | ||
function updateHiddenSubscribe() { | ||
const wrapper = (containerRef && containerRef.current) || imageWrapperRef.current; | ||
if (wrapper && visibility === 'on-image-hover') { | ||
const onMouseOver = () => setHidden(false); | ||
const onMouseOut = () => setHidden(true); | ||
|
||
wrapper.addEventListener('mouseover', onMouseOver); | ||
wrapper.addEventListener('mouseout', onMouseOut); | ||
|
||
return () => { | ||
wrapper.removeEventListener('mouseover', onMouseOver); | ||
wrapper.removeEventListener('mouseout', onMouseOut); | ||
}; | ||
} | ||
return; | ||
}, | ||
[visibility, imageWrapperRef, containerRef], | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не думал по поводу такой реализации?
<ImageBase src="">
<ImageBase.Overlay
// можно без disableInteractive если хочется клик на весь блок
disableInteractive
>
<ImageBase.OverlayFloatElement position="top-start">
<Button>Кнопка</Button>
</ImageBase.OverlayFloatElement>
</ImageBase.Overlay>
</ImageBase>
- разрешит непонятку по поводу совместного использования с
ImageBase.Overlay
, в целом идеологически будет корректней; - не нужно будет вещать события
mouse
, т.к.ImageBase.Overlay
уже умеет в это – параметрыvisibility
иcontainerRef
можно будет удалить
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Точно ли будет удобно пользоваться такой API. Для каждого элемента, надо будет создавать OverlayFloatElement обернутый в Overlay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inomdzhon Попробовал реализовать через Overlay
и понял, что будут проблемы, когда нужно будет спозиционировать несколько элементов на картинке, потому что overlay
растягивается на весь размер картинки. И получается, что последний отрендеренный элемент перекроет другие и не появится при наведении на картинку
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
имеешь ввиду про вот такой кейс?
<ImageBase src="">
<ImageBase.Overlay
// можно без disableInteractive если хочется клик на весь блок
disableInteractive
>
<ImageBase.OverlayFloatElement position="top-start">
<Button>👍</Button>
</ImageBase.OverlayFloatElement>
<ImageBase.OverlayFloatElement position="top-start">
<Button>⚙️</Button>
</ImageBase.OverlayFloatElement>
</ImageBase.Overlay>
</ImageBase>
если да, то это скорее вот так должно быть свёрстано
<ImageBase src="">
<ImageBase.Overlay
// можно без disableInteractive если хочется клик на весь блок
disableInteractive
>
<ImageBase.OverlayFloatElement position="top-start">
<Flex>
<Flex.Item><Button>👍</Button></Flex.Item>
<Flex.Item><Button>⚙️</Button></Flex.Item>
</Flex>
</ImageBase.OverlayFloatElement>
</ImageBase.Overlay>
</ImageBase>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
да, так будет работать, но не во всех кейсах. Если понадобится сделать так, чтобы один элемент был виден всегда, а другой только при наведении на картинку. Тогда по идее верстка будет следующая:
<ImageBase src="">
<ImageBase.Overlay
// можно без disableInteractive если хочется клик на весь блок
disableInteractive
visibility="on-hover"
>
<ImageBase.OverlayFloatElement position="top-start">
<Flex.Item><Button>👍</Button></Flex.Item>
</ImageBase.OverlayFloatElement>
</ImageBase.Overlay>
<ImageBase.Overlay
// можно без disableInteractive если хочется клик на весь блок
disableInteractive
visibility="always"
>
<ImageBase.OverlayFloatElement position="bottom-start">
<Flex.Item><Button>⚙️</Button></Flex.Item>
</ImageBase.OverlayFloatElement>
</ImageBase.Overlay>
</ImageBase>
При этом второй элемент перекроет первый и первый не появится при наведении
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если понадобится сделать так, чтобы один элемент был виден всегда, а другой только при наведении на картинку.
@MrZillaGold у вас есть такой кейс?
Кстати, к этой задаче нужно подключить дизайн (@Zaycevq или @qurle) – на уровне дизайн-системы нужно зафиксировать поведение
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inomdzhon Обсудили с дизайном, могут быть кейсы с несколькими элементами
.../vkui/src/components/ImageBase/ImageBasePositionedComponent/ImageBasePositionedComponent.tsx
Outdated
Show resolved
Hide resolved
# Conflicts: # packages/vkui/src/components/ImageBase/ImageBase.tsx
packages/vkui/src/components/ImageBase/ImageBaseFloatElement/ImageBaseFloatElement.tsx
Outdated
Show resolved
Hide resolved
packages/vkui/src/components/ImageBase/ImageBaseFloatElement/ImageBaseFloatElement.tsx
Outdated
Show resolved
Hide resolved
…image-hover' to 'on-hover'
/** | ||
* Позиция компонента относительно родителя | ||
*/ | ||
position: FloatElementPlacement | FloatElementPosition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
что на счёт вместо FloatElementPosition
использовать синтаксис <x> <y>
+ вынести это в отдельный параметрм offset
? Выбираешь где будет элемент, а если нужно, добавляешь смещение
<ImageBaseFloatElement position="top" offset="5% 5%">
<Icon24Add />
</ImageBaseFloatElement>
<ImageBaseFloatElement position="middle-start" offset="0 10px">
<Icon24Add />
</ImageBaseFloatElement>
или как в lib/floating/useFloatingMiddlewaresBootstrap/index.ts сделать два отдельных параметра offsetByMainAxis
и offsetByCrossAxis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По сути у нас же есть свойства blockIndent
и inlineIndent
, которые позволяют устанавливать отступы. Как будто данного функционала должно хватить. Также свойства blockIndent
и inlineIndent
работают с токенами дизайн системы, что добавляет системности
Описание
Нужно добавить сабкомпонент для компонента
Image
для позиционированирования компонентов внутриImage
.Изменения
ImageBaseFloatElement
для абсолютного позиционирования компонентов в компоненте Image.Release notes
Новые компоненты
Image.FloatElement
для позиционирования компонента относительно картинки